Avoid loading all tasks to validate a Run#task_name
#866
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, creating a `Run` record validates that its `task_name` belongs to the list returned by `Task.available_tasks`. Generating this list requires the gem to browse through the task namespace(s) to find and load all tasks in the application (see `Task#load_constants`). Although in most situations this should not be a problem, the Shopify monolith defines a relatively large amount of maintenance tasks, which take a considerable amount of time to load. Loading all tasks is only absolutely necessary when the application wants to display a list of all tasks. In any other scenario, for example testing a single task, it should not be necessary to load all tasks. This commit makes that possible by making use of `Task.named` to confirm that the task name is valid. Because `available_tasks` relies on `.descendants`, and `Task.named` uses ` < Task` to test inheritance, I don't think this will have any unintended effect on the gem's behaviour.
davidstosik
force-pushed
the
sto/task-name-validate
branch
from
81ee1c9
to
292ebc3
Compare
etiennebarrie
approved these changes
Aug 18, 2023
davidstosik
added a commit
that referenced
this pull request
Aug 18, 2023
In ordered to be displayed in the UI, tasks have to be loaded by the time `MaintenanceTasks::Task.descendants` is called. To achieve that, the `Task.load_constants` method was previously implemented, and called from `Task.available_tasks`. A few inconvenients of that method are: - It could easily be abused and used in lower levels of the gem, where ideally it should ne be necessary to load all tasks to achieve the gem's goal. See for example my previous PR [Avoid loading all tasks to validate a `Run#task_name`](#866). - It can introduce unnecessary overhead. For example, displaying the CLI's basic help message with `maintenance_tasks help` will necessitate all tasks to be loaded (even though the help message does not display anything related to the available tasks), and this loading time will delay the appearance of that help message. Similarly, a single test case that want to test a single task's behaviour, while never displaying the whole list of available tasks, would be negatively impacted by a misuse of `Task.available_tasks`. - It does not allow an application to define an alternative way to load all constants. As a matter of fact, the Shopify Core monolith monkey-patched that method to allow multiple "task modules" instead of a single one. With a configurable `task_loader`, monkey-patching would not be necessary. (Which is good. 🙈) This PR introduces `MaintenanceTasks.task_loader`, a configuration that allows applications to change the logic applied to ensure all task are loaded before being displayed. As a bonus: - it changes how the CLI generates the long description containing all task names, so that tasks get loaded at the last minute, only when necessary - it gets rid of `Task.available_tasks` (its only use was for UI purposes, so I changed it to a direct call to `Task.descendants`) - it introduces some tests of the loading behaviour
davidstosik
added a commit
that referenced
this pull request
Aug 18, 2023
In ordered to be displayed in the UI, tasks have to be loaded by the time `MaintenanceTasks::Task.descendants` is called. To achieve that, the `Task.load_constants` method was previously implemented, and called from `Task.available_tasks`. A few inconvenients of that method are: - It could easily be abused and used in lower levels of the gem, where ideally it should ne be necessary to load all tasks to achieve the gem's goal. See for example my previous PR [Avoid loading all tasks to validate a `Run#task_name`](#866). - It can introduce unnecessary overhead. For example, displaying the CLI's basic help message with `maintenance_tasks help` will necessitate all tasks to be loaded (even though the help message does not display anything related to the available tasks), and this loading time will delay the appearance of that help message. Similarly, a single test case that want to test a single task's behaviour, while never displaying the whole list of available tasks, would be negatively impacted by a misuse of `Task.available_tasks`. - It does not allow an application to define an alternative way to load all constants. As a matter of fact, the Shopify Core monolith monkey-patched that method to allow multiple "task modules" instead of a single one. With a configurable `task_loader`, monkey-patching would not be necessary. (Which is good. 🙈) This PR introduces `MaintenanceTasks.task_loader`, a configuration that allows applications to change the logic applied to ensure all task are loaded before being displayed. As a bonus: - it changes how the CLI generates the long description containing all task names, so that tasks get loaded at the last minute, only when necessary - it gets rid of `Task.available_tasks` (its only use was for UI purposes, so I changed it to a direct call to `Task.descendants`) - it introduces some tests of the loading behaviour
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, creating a
Runrecord validates that itstask_namebelongs to the list returned byTask.available_tasks.Generating this list requires the gem to browse through the task namespace(s) to find and load all tasks in the application (see
Task#load_constants).Although in most situations this should not be a problem, the Shopify monolith defines a relatively large amount of maintenance tasks, which take a considerable amount of time to load.
Loading all tasks is only absolutely necessary when the application wants to display a list of all tasks. In any other scenario, for example testing a single task, it should not be necessary to load all tasks.
This commit makes that possible by making use of
Task.namedto confirm that the task name is valid.Because
available_tasksrelies on.descendants, andTask.nameduses< Taskto test inheritance, I don't think this will have any unintended effect on the gem's behaviour.